fold 'kola testiso' into 'kola run'#4377
fold 'kola testiso' into 'kola run'#4377nikita-dubrovskii wants to merge 26 commits intocoreos:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request is a large but valuable refactoring that folds the kola testiso command into kola run. This improves the consistency and maintainability of the test suite. The implementation moves the ISO test logic into a new mantle/kola/tests/iso package and refactors the QEMU cluster platform API to be more extensible and modular, which is a great improvement.
I've found a few issues that could be improved:
- There are a couple of places where
panic()is used inside a goroutine for error handling, which can crash the entire test runner. It would be better to handle these errors more gracefully, for example by logging them. - There is a potential goroutine leak in the
awaitCompletionhelper function due to atime.Sleepthat doesn't respect context cancellation. - A file path is constructed using
strings.Replace, which is fragile. Using thepath/filepathpackage would be more robust.
Overall, this is a solid refactoring. My comments are focused on improving robustness and resource management.
|
It's not yet 100% ready - i'm reworking metal PXE&ISO installation, so those tests still rely on code from |
9499239 to
9816db6
Compare
|
Finished metal PXE&ISO installation. Almost ready for review. |
34d0988 to
ac38a76
Compare
|
s390x run: |
ac38a76 to
aa167c6
Compare
|
@nikita-dubrovskii this is an incredible amount of work. Thank you for taking this on. This does make it hard to grok all in one go, though. I'm still struggling a bit with the first commit even. As I go I'm questioning old code that we have and I want to challenge if some of it is necessary. I broke out some initial commits that challenge the original code (and also a few commits from this PR) into #4486 I also rebased the commits from this PR on top of that in https://github.com/dustymabe/coreos-assembler/commits/dusty-kola-testiso-fold/ if you're interested in a rebased version of this PR. Let me know what you think. |
|
@dustymabe Thx, checked your PR. Looks sane to me, and many thanks for better commit messages, describing purpose of changes. The only thing holding me back is dealing with rebase&merge conflicts, but not a big deal. |
Thanks!
Those should be handled already over in https://github.com/dustymabe/coreos-assembler/commits/dusty-kola-testiso-fold/ Once #4486 merges (if it gets approved) I can push that rebase over here if you like. |
Yes, please. |
|
I did some more looking at this today. The overall strategy, not just the first commit. I'm still forming opinions and not really ready to share anything yet. |
bd58831 to
5369e81
Compare
5369e81 to
7582d66
Compare
7582d66 to
d75f86b
Compare
d75f86b to
925d8a3
Compare
…ation Introduce MachineBuilder struct to allow callers to customize QEMU machine creation by providing hooks for builder initialization, disk setup, network configuration. This will be used to fold 'testiso' tests into the standard 'kola run', where custom machine creation logic is needed for ISO-based installations.
This migrates the iso-live-login tests from the standalone 'kola testiso' command into the standard 'kola run' framework, making them available alongside other kola tests. Changes: - Moved iso-live-login* tests implementation from cmd/kola/testiso.go to mantle/kola/tests/iso/live-login.go - Removed 4k test variants (iso-live-login.4k.uefi) as they were non-functional for this test type Why 4k tests were dropped: The 4k flag in testiso only affects disk sector size during disk creation. However, the live-login test boots directly from ISO without creating any disks, so the 4k flag had no effect. The 4k test variants were redundant and ran identical code to their non-4k counterparts.
Relocate metal.go to machine/qemu/ directory where it belongs, as it contains QEMU-specific code for ISO installations and is only used by ISO tests, not as a general platform implementation.
Temporarily consolidate InstalledMachine into machine struct to simplify the codebase during the testiso migration. This prepares for migrating ISO installation tests from the testiso command into the standard 'kola run' test framework. This merge is temporary and will be refactored once the migration is complete.
Split the monolithic live-iso.go file into several focused files where each file contains a specific set of tests: - live-pxe.go: PXE network boot tests - live-iscsi.go: iSCSI boot and installation tests - live-as-disk.go: ISO-as-disk installation tests - common.go: Shared test utilities and helper functions This improves code organization and makes it easier to locate and maintain specific test suites.
Add journal dumping to signalFailureUnit to capture diagnostic information when the system enters emergency.target during ISO tests. After switch root occurs during live ISO testing, coreos-installer runs from the real root filesystem rather than the initramfs, which means ignition-virtio-dump-journal.service is no longer enabled. This change ensures that when emergency.target is reached, the journal is still dumped to the virtio port, allowing platform.StartMachine to capture errors. The unit now checks if ignition-virtio-dump-journal.service is enabled, and if not, manually executes the journal dump script from the dracut emergency shell setup module.
Move the 'inst-insecure' and 'pxe-kargs' parameters from local test options to global QEMUOptions structure, making them available as command-line flags for all ISO tests: - --inst-insecure: Skip signature verification on metal image - --pxe-kargs: Additional kernel arguments for PXE boot tests
Migrate the last remaining test from the legacy 'kola testiso' command into the standard 'kola run' This completes the migration of all ISO tests and removes the now-obsolete testiso.go file.
Extract duplicate completion channel reading logic into a reusable CheckTestOutput helper function. This reduces code duplication across iso tests.
The kola test harness already performs console and journal checks
automatically after each test completes, making the duplicate checks
in awaitCompletion unnecessary.
Call flow in mantle/kola/harness.go:
```
runProvidedTests
-> runTest
-> handleConsoleChecks("console", ...)
-> CheckConsole
-> handleConsoleChecks("journal", ...)
-> CheckConsole
```
Update the test to use the existing MachineBuilder API after the testiso migration.
Also remove the 4k sector size test variant as it's technically impossible to configure. The AddIso() function and iso-as-disk implementation do not support sector size configuration - the ISO is attached as a raw, readonly device without any sector size options. The Disk.SectorSize field only applies to regular disk images added via AddDisk() or AddDisksFromSpecs(), not to ISOs attached via AddIso(). Since iso-as-disk tests boot directly from the ISO file (not a disk image), sector size configuration is not applicable.
Only wait for SSH address when usermode networking is enabled. This allows ISO tests without networking to proceed without unnecessary SSH address waiting.
Add Instance() method to expose the underlying QemuInstance from a platform.Machine. This allows tests to access QEMU-specific functionality not available through the generic interface.
Extract the Ignition config path logic into a reusable IgnitionPath() method that returns the path where the Ignition config will be written. This allows callers to get the Ignition config path before rendering, which is useful for tests that need to create symlinks or perform other operations on the config file path before the machine is started.
Allow callers to pass nil userdata/config when Ignition configuration is not needed, such as in PXE boot scenarios where configuration is provided through alternative means.
Remove deprecated machine struct fields and methods that are no longer needed after the migration all ISO tests under kola.
Replace enableUefi and enableUefiSecure boolean flags with a single firmware string field in IsoTestOpts for simpler and more consistent firmware configuration across all ISO tests.
925d8a3 to
f4bfdd3
Compare
|
@nikita-dubrovskii: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This huge PR folds all
testisotests:#3989